-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VReplication: Support automatically replacing auto_inc cols with sequences #16860
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
82a1670
to
eec4b9c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16860 +/- ##
========================================
Coverage 69.42% 69.43%
========================================
Files 1571 1571
Lines 203304 203433 +129
========================================
+ Hits 141148 141249 +101
- Misses 62156 62184 +28 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
eec4b9c
to
33b4cc9
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
1c85d2a
to
e56dd5b
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
f462d86
to
21b2871
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
21b2871
to
f5eb4e2
Compare
5f28cfa
to
ea0d3ad
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
ea0d3ad
to
b110120
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
669a5f7
to
e89bbf7
Compare
go/cmd/vtctldclient/command/vreplication/movetables/movetables.go
Outdated
Show resolved
Hide resolved
proto/vtctldata.proto
Outdated
bool strip_sharded_auto_increment = 2; | ||
// keyspace and optionally replace them with vschema AutoIncrement | ||
// definitions. | ||
ShardedAutoIncrementHandling strip_sharded_auto_increment = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename this to sharded_auto_increment_handling
or similar. Renaming proto fields does not break compatibility in anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, when did we add this field? If it was within v21, we are fine. otherwise we'd have to reserve this and add a new field because we are changing the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on renaming. Will do. That field was added in v20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then changing bool to enum is theoretically a breaking change. BUT, since this is between vtctldclient and vtctld, we can assume that their versions should match and not worry about reserving the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The potential problem with renaming the proto field is that the field name is used when marshalling/unmarshalling the field to protojson or prototext. So it would mean that we do not read the value properly for old/existing workflows ... which is fine because in the older versions any actions taken from that value are done when creating the workflow and the value is not needed/used after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to address this here: 733198b
@@ -209,16 +209,25 @@ message Shard { | |||
topodata.Shard shard = 3; | |||
} | |||
|
|||
enum ShardedAutoIncrementHandling { | |||
LEAVE = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of calling this NONE
instead of LEAVE
? Meaning we do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. If you prefer that then I'm fine changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I remember that this is what I had started with, but you cannot:
vtctldata.proto:213:3: "NONE" is already defined in "vtctldata".
See: protocolbuffers/protobuf#5425
So we can leave it at LEAVE, or change it to e.g. NO_HANDLING etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, let's "leave" it.
targetVSchema *vschema.Keyspace | ||
options *vtctldatapb.WorkflowOptions | ||
want map[string]*sequenceMetadata | ||
expectSourceApplySchemaRequest *applySchemaRequestResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there seems to be a mismatch between the field name and type. Should the field name also be ...Response
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets a bit long, and this is test-only code where you can clearly see the type, but I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh you mean if you call it ...RequestResponse
? I think expectApplySchemaResponse
is adequate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it here: 733198b
@@ -287,7 +287,7 @@ func AddCommonSwitchTrafficFlags(cmd *cobra.Command, initializeTargetSequences b | |||
cmd.Flags().BoolVar(&SwitchTrafficOptions.DryRun, "dry-run", false, "Print the actions that would be taken and report any known errors that would have occurred.") | |||
cmd.Flags().BoolVar(&SwitchTrafficOptions.Force, "force", false, "Force the traffic switch even if some potentially non-critical actions cannot be performed; for example the tablet refresh fails on some tablets in the keyspace. WARNING: this should be used with extreme caution and only in emergency situations!") | |||
if initializeTargetSequences { | |||
cmd.Flags().BoolVar(&SwitchTrafficOptions.InitializeTargetSequences, "initialize-target-sequences", false, "When moving tables from an unsharded keyspace to a sharded keyspace, initialize any sequences that are being used on the target when switching writes.") | |||
cmd.Flags().BoolVar(&SwitchTrafficOptions.InitializeTargetSequences, "initialize-target-sequences", false, "When moving tables from an unsharded keyspace to a sharded keyspace, initialize any sequences that are being used on the target when switching writes. If the sequence table is not found AND the sequence table reference was fully qualified OR a value was specified for --global-keyspace, then we will attempt to create each sequence table in that keyspace if it doesn't exist.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear exactly what the boolean logic here means to someone who doesn't already know it.
I'm reading it as
(IF the sequence table is not found) AND ((the sequence table reference was fully qualified) OR (a value was specified for --global-keyspace))
but it should be rewritten to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can think about how to make it more clear. It reads pretty clear to me but I'll think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to address this here: 733198b
The change is subtle, but I think it's quite clear.
@@ -40,6 +40,8 @@ var ( | |||
NoRoutingRules bool | |||
AtomicCopy bool | |||
WorkflowOptions vtctldatapb.WorkflowOptions | |||
// This maps to a WorkflowOptions.StripShardedAutoIncrement ENUM value. | |||
StripShardedAutoIncrementStr string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with the proto name change, we should change this name. It is confusing to call it Strip
when it could be NONE/REMOVE/REPLACE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some nuance here that I missed during review. So maybe you just need to explain what I'm missing :)
Quoting Rohit:
Maybe consider naming the StripShardedAutoIncrement differently for the string and enum representations. Also adding a comment to note that the first one is from the CLI flags and second to set in the _vt.vreplication row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'll rename it to ShardedAutoIncrementHandling[Str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to address this here: 733198b
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
We'll run the website docs update for the vtctld command reference once before release. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
|
||
## <a id="major-changes"/>Major Changes | ||
## <a id="major-changes"/>Major Changes</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was adding </a>
to every section heading done by your editor? Seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it as w/o it the content is messed up in some readers (the preview one I use). It’s proper to end it.
changelog/21.0/21.0.0/summary.md
Outdated
|
||
### <a id="auto-replace-mysql-autoinc-with-seq"/>Automatically Replace MySQL auto_increment Clauses with Vitess Sequences</a> | ||
|
||
When migrating tables from an unsharded keyspace to a sharded one using the [VReplication `MoveTables` command](https://vitess.io/docs/reference/vreplication/movetables/), we now support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too verbose for a summary. Can you please condense it into a short paragraph? No need to give all the background, just describe the feature briefly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind if we add something to the summary we think it’s worth highlighting. At a minimum we should then describe what the thing is that we’re highlighting and explain why we think it’s worth highlighting. Which to me includes the context for what it is, what it is, why it’s important, when you'd use it, and how to use it. Two paragraphs is not a lot IMO and if you look at this and previous summary docs multiple paragraphs is quite common (just look at the one right above this). That being said, I will try and condense it and eliminate some content that isn’t strictly necessary for the stated objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a docs PR for this: vitessio/website#1859
Once that's merged this can become a short paragraph that links to this for additional details: https://deploy-preview-1859--vitess.netlify.app/docs/21.0/reference/vreplication/movetables/#auto-increment-handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 3ceb253 Thanks!
Signed-off-by: Matt Lord <mattalord@gmail.com>
* Update reference docs for vitessio/vitess#16860 Signed-off-by: Matt Lord <mattalord@gmail.com> * Add new section to the movetables reference page Signed-off-by: Matt Lord <mattalord@gmail.com> * Add keyspace concept link Signed-off-by: Matt Lord <mattalord@gmail.com> * Changes from self review Signed-off-by: Matt Lord <mattalord@gmail.com> * Correct keyspace concept link Signed-off-by: Matt Lord <mattalord@gmail.com> * Minor tweak Signed-off-by: Matt Lord <mattalord@gmail.com> * Unify formatting Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Description
Since #15679 we automatically strip any MySQL
auto_increment
clauses on tables that are being moved (using theMoveTables
command) from an unsharded keyspace to a sharded one. And since #13656 we support initializing any sequences in the target keyspace onSwitchTraffic
.In this PR, we build upon both of those items of work to support automatically creating a Vitess Sequence for each if those tables that HAD an
auto_increment
clause if one does not already exist. This requires the use of two flags:- That we want to replace any removed MySQL auto_increment clauses with vschema AutoIncrement definitions using
--sharded-auto-increment-handling=replace
- An unsharded keyspace in
--global-keyspace
that we can then later use to create the sequence tables in if they don't already exist (they don't in the manual test below)--initialize-target-sequences
flag, which will create a missing sequence table when possibleManual test
Results:
The
strip_sharded_auto_increment
field was renamed tosharded_auto_increment_handling
— which is upgrade/downgrade safe with protobufs as field indexes are used — and the type of the proto field was changed frombool
to anenum
/int32
to support the new REPLACE option. This is upgrade/downgrade safe because the wire format for bool is avarint
with 0 being false and 1 being true — with the deserialization ending up 0 = false > 0 = true. To demonstrate:And using the manual test case from above but using binaries built on
main
with avtctldclient
binary from the PR branch:So we can see that the auto_increment clauses were still removed.
Related Issue(s)
Checklist